-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang-tidy][NFC] Enable "HeaderFilterRegex" in clang-tidy codebase #167020
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| @@ -1,4 +1,6 @@ | |||
| InheritParentConfig: true | |||
| HeaderFilterRegex: 'clang-tools-extra/clang-tidy' | |||
| ExcludeHeaderFilterRegex: 'include-cleaner|clang-query' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to do this because I got errors from files like this:
llvm/llvm-project/clang-tools-extra/clang-tidy/misc/../../include-cleaner/include/clang-include-cleaner/Types.h:22:9
So it matched on regex clang-tools-extra/clang-tidy but actually it's header from another subproject.
If anyone have better ideas - please tell!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the relevant code:
llvm-project/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
Lines 590 to 594 in 7734276
| const StringRef FileName(File->getName()); | |
| LastErrorRelatesToUserCode = LastErrorRelatesToUserCode || | |
| Sources.isInMainFile(Location) || | |
| (getHeaderFilter()->match(FileName) && | |
| !getExcludeHeaderFilter()->match(FileName)); |
Maybe we should normalize
FileName before matching on it? i.e.:
clang-tools-extra/clang-tidy/misc/../../include-cleaner/include/clang-include-cleaner/Types.h
->
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.hThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, the reason we have these relative paths in the first place is because include-cleaner forces us to do this:
| include_directories(BEFORE "${CMAKE_CURRENT_SOURCE_DIR}/../../include-cleaner/include") |
...which is just really ugly. I've opened #167110 to fix it. (It happens to also fix this problem, but I think there's an underlying problem on our end too.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI same happens with clang-query headers that also emit relative paths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's right, that PR doesn't fully fix this problem then
EugeneZelenko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK for me, but please wait for other reviewers.
|
I think we can proceed to merge "as is" given CMake changes will take time to be merged, and they won't fix the whole problem. WDYT? |
|
That sounds reasonable, we can merge this |
No description provided.